fix(zod): parser generation when content-type contains charset precision - #3404
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughNormalize content-type keys by trimming parameters before matching JSON or multipart/form-data in parseBodyAndResponse; preserve schema dereference, array handling, and Zod generation paths. Add getSingleResponse to pick one response (200 → 2XX → 2xx). Add charset-qualified tests. ChangesCharset-normalized media-type handling
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds support for OpenAPI media types that include parameters (e.g., ; charset=utf-8) when generating Zod schemas, and introduces a regression test to ensure parity with existing content-type handling expectations.
Changes:
- Update content-type selection logic to match
application/json*andmultipart/form-data*entries (including parameterized variants). - Add a Zod generation test covering
multipart/form-data; charset=utf-8request bodies andapplication/json; charset=utf-8responses.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| packages/zod/src/zod.test.ts | Adds a regression test for multipart/form-data request bodies keyed with ; charset=utf-8. |
| packages/zod/src/index.ts | Updates request/response content-type selection to accept parameterized JSON and multipart media types. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/zod/src/zod.test.ts (1)
6357-6366: ⚡ Quick winAdd an assertion for charset-qualified response parsing in this scenario.
At Line 6362, this test reuses
zodOverridewithresponse: false, so it never validates the response-side charset path even though the fixture includes it. Add a response-enabled variant (or a second assertion block) to verify parser generation forapplication/json; charset=utf-8responses.Also applies to: 6375-6390
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/zod/src/zod.test.ts` around lines 6357 - 6366, The test currently calls generateZod with zodOverride that has response: false so it never exercises the response-side charset handling; add a second call (or duplicate the existing block) invoking generateZod with response enabled (e.g., a copy of zodOverride but with response: true) for the same operationName/uploadForm and schema/testOutput, then add an assertion that the generated parser code (the returned result) contains handling for "application/json; charset=utf-8" (or the exact charset-qualified content-type string) to validate the response-side charset-qualified parsing path; reference generateZod, zodOverride, operationName/uploadForm and the result variable when adding the new assertion.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/zod/src/index.ts`:
- Around line 1339-1345: The media-type matching using startsWith is fragile;
normalize and compare base media types instead: iterate contentEntries (from
resolvedRef.content) and for each contentType split off any parameters at the
first ';', trim and lowercase the base type, then set jsonContent when base ===
'application/json' and formDataContent when base === 'multipart/form-data' so
matches like 'Application/JSON; charset=UTF-8' succeed and unintended prefixes
are not matched.
---
Nitpick comments:
In `@packages/zod/src/zod.test.ts`:
- Around line 6357-6366: The test currently calls generateZod with zodOverride
that has response: false so it never exercises the response-side charset
handling; add a second call (or duplicate the existing block) invoking
generateZod with response enabled (e.g., a copy of zodOverride but with
response: true) for the same operationName/uploadForm and schema/testOutput,
then add an assertion that the generated parser code (the returned result)
contains handling for "application/json; charset=utf-8" (or the exact
charset-qualified content-type string) to validate the response-side
charset-qualified parsing path; reference generateZod, zodOverride,
operationName/uploadForm and the result variable when adding the new assertion.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 527a6045-ce1e-4fce-b8e8-a97eab2c041b
📒 Files selected for processing (2)
packages/zod/src/index.tspackages/zod/src/zod.test.ts
melloware
added
zod
melloware
left a comment
melloware
left a comment
There was a problem hiding this comment.
Please see feedback from AI it made good suggestions
|
@pierre-isabel-bbc also are you interested in fixing this issue while you are in there: #3285 |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/zod/src/index.ts (1)
1424-1427:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMake media-type normalization case-insensitive to avoid missed parser generation.
At Line 1427, comparison is still case-sensitive. Valid keys like
Application/JSON; charset=UTF-8won’t match and will skip schema generation.Suggested fix
const isMediaType = (expectedContentType: string) => ([contentType]: [string, object]): boolean => - contentType.split(';')[0].trim() === expectedContentType; + contentType.split(';')[0].trim().toLowerCase() === + expectedContentType.toLowerCase();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/zod/src/index.ts` around lines 1424 - 1427, The media-type comparison in isMediaType is case-sensitive and can miss matches like "Application/JSON; charset=UTF-8"; update the check inside isMediaType to normalize both the incoming contentType and the expectedContentType to the same case (e.g., toLowerCase()) after trimming and splitting so the comparison is case-insensitive; locate the isMediaType function and apply normalization to the contentType base and expectedContentType before comparing.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@packages/zod/src/index.ts`:
- Around line 1424-1427: The media-type comparison in isMediaType is
case-sensitive and can miss matches like "Application/JSON; charset=UTF-8";
update the check inside isMediaType to normalize both the incoming contentType
and the expectedContentType to the same case (e.g., toLowerCase()) after
trimming and splitting so the comparison is case-insensitive; locate the
isMediaType function and apply normalization to the contentType base and
expectedContentType before comparing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: df00ccde-49e2-44f8-8630-493fd259ce00
📒 Files selected for processing (2)
packages/zod/src/index.tspackages/zod/src/zod.test.ts
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/zod/src/index.ts (1)
1341-1349:⚠️ Potential issue | 🟠 Major | ⚡ Quick winJSON media-type matcher still misses valid vendor types and case variants.
At Line 1347 and Line 1436,
application/vnd.api+jsonand mixed-case variants can still fail matching, so response/body parsers may be skipped for valid JSON media keys.Suggested fix
-const isMediaType = - (pattern: string) => - ([contentType]: [string, object]): boolean => - new RegExp(pattern).test(contentType.split(';')[0].trim()); +const isMediaType = + (pattern: RegExp | string) => + ([contentType]: [string, object]): boolean => { + const base = contentType.split(';', 1)[0].trim().toLowerCase(); + const regex = + pattern instanceof RegExp ? pattern : new RegExp(pattern, 'i'); + return regex.test(base); + };- const jsonContent = contentEntries.find( - isMediaType( - // application/json - // application/geo+json - // application/ld+json - // application/manifest+json - String.raw`^application\/([\w-]+\+)?json$`, - ), - ); + const jsonContent = contentEntries.find( + // application/json and application/*+json (RFC token chars) + isMediaType(/^application\/(?:json|[a-z0-9!#$&^_.+-]+\+json)$/i), + );Also applies to: 1433-1436
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/zod/src/index.ts` around lines 1341 - 1349, The current regex passed to isMediaType (used to compute jsonContent and the other identical match later) misses vendor prefixes and is case-sensitive; update the pattern to allow vendor-type prefixes (e.g., vnd.<name>+ or any +suffix) and enable case-insensitive matching when calling isMediaType so media-types like "application/vnd.api+json" and mixed-case variants match; apply the same change to both places where the regex is used (the jsonContent computation and the second identical occurrence).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@packages/zod/src/index.ts`:
- Around line 1341-1349: The current regex passed to isMediaType (used to
compute jsonContent and the other identical match later) misses vendor prefixes
and is case-sensitive; update the pattern to allow vendor-type prefixes (e.g.,
vnd.<name>+ or any +suffix) and enable case-insensitive matching when calling
isMediaType so media-types like "application/vnd.api+json" and mixed-case
variants match; apply the same change to both places where the regex is used
(the jsonContent computation and the second identical occurrence).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0dc208f6-afc2-4b2b-a793-0750e9ca34b1
📒 Files selected for processing (2)
packages/zod/src/index.tspackages/zod/src/zod.test.ts
@melloware Hey! Done in this commit: 4e8b197 |
melloware
left a comment
melloware
left a comment
There was a problem hiding this comment.
looks like now you need to regen snapshots? the Snapshot tests are failing
121275f to
a998d35
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/zod/src/zod.test.ts (1)
6409-6489: ⚡ Quick winAdd a dotted vendor JSON media-type regression case.
Please add a case like
application/vnd.api+json(optionally with charset) so valid vendor subtypes are covered, not onlygeo+json/manifest+json.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/zod/src/zod.test.ts` around lines 6409 - 6489, The test "json exotic content type: comprehensive content type handling" is missing a dotted vendor JSON media-type case; update the requestBody and response content maps to include 'application/vnd.api+json' (and one variation with a charset like 'application/vnd.api+json; charset=utf-8') alongside 'application/geo+json' and 'application/manifest+json' so vendor subtypes are covered; ensure the test still calls generateZod (the generateZod invocation and override remain unchanged) and keep the expected Zod outputs (UploadFormBody / UploadFormResponse) the same since the schema is identical, only add the new content-type entries in the spec object.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/zod/src/index.ts`:
- Around line 1341-1348: The media-type regex used when computing jsonContent
(contentEntries.find(... isMediaType(...))) disallows dots in vendor subtype
names (so types like application/vnd.api+json are rejected); update the pattern
passed to isMediaType (the String.raw`^application\/([\w-]+\+)?json$` literal)
to allow dots (and keep existing word, plus and hyphen chars) in the subtype
token (e.g. use a character class that includes '.' such as [\w.-] or [\w.+-])
so vendor subtypes like vnd.api+json are accepted.
---
Nitpick comments:
In `@packages/zod/src/zod.test.ts`:
- Around line 6409-6489: The test "json exotic content type: comprehensive
content type handling" is missing a dotted vendor JSON media-type case; update
the requestBody and response content maps to include 'application/vnd.api+json'
(and one variation with a charset like 'application/vnd.api+json;
charset=utf-8') alongside 'application/geo+json' and 'application/manifest+json'
so vendor subtypes are covered; ensure the test still calls generateZod (the
generateZod invocation and override remain unchanged) and keep the expected Zod
outputs (UploadFormBody / UploadFormResponse) the same since the schema is
identical, only add the new content-type entries in the spec object.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8ae506c3-98e0-4508-a78c-86a568734330
📒 Files selected for processing (2)
packages/zod/src/index.tspackages/zod/src/zod.test.ts
| const jsonContent = contentEntries.find( | ||
| isMediaType( | ||
| // application/json | ||
| // application/geo+json | ||
| // application/ld+json | ||
| // application/manifest+json | ||
| String.raw`^application\/([\w-]+\+)?json$`, | ||
| ), |
There was a problem hiding this comment.
JSON media-type matcher still misses valid vendor subtypes.
The current pattern rejects valid JSON media types like application/vnd.api+json (dot in subtype), so schema generation can still be skipped for legitimate responses/requests.
Suggested fix
const jsonContent = contentEntries.find(
isMediaType(
// application/json
// application/geo+json
// application/ld+json
// application/manifest+json
- String.raw`^application\/([\w-]+\+)?json$`,
+ // application/vnd.api+json (and other valid vendor subtypes)
+ String.raw`^application\/([^/;]+\+)?json$`,
),
);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/zod/src/index.ts` around lines 1341 - 1348, The media-type regex
used when computing jsonContent (contentEntries.find(... isMediaType(...)))
disallows dots in vendor subtype names (so types like application/vnd.api+json
are rejected); update the pattern passed to isMediaType (the
String.raw`^application\/([\w-]+\+)?json$` literal) to allow dots (and keep
existing word, plus and hyphen chars) in the subtype token (e.g. use a character
class that includes '.' such as [\w.-] or [\w.+-]) so vendor subtypes like
vnd.api+json are accepted.
a998d35 to
1fc23d7
Compare
1fc23d7 to
431e3fc
Compare
Reproduction
OpenAPI contract Content-Type may contain
charsetreference, like in the example below:The issue was that parser for 200 was generated, but not for 400 because of the
charset=utf-8precision.What this PR does
Summary by CodeRabbit
Bug Fixes
Tests